-
Notifications
You must be signed in to change notification settings - Fork 36
FIX: Cursor.describe invalid data #355
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes issue #352 by implementing two main improvements: (1) correcting cursor.description to return SQL type codes (integers) instead of Python type objects, ensuring DB-API 2.0 specification compliance, and (2) adding support for SQL Server spatial data types (geography, geometry, hierarchyid) by handling the SQL_SS_UDT type code (-151).
Key changes:
- Fixed
cursor.description[i][1]to return SQL type integer codes (e.g., 4, -9) instead of Python types (int, str) per DB-API 2.0 spec - Added SQL_SS_UDT (-151) support for SQL Server User-Defined Types including spatial data types
- Updated output converter lookup to use SQL type codes consistently throughout the codebase
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| mssql_python/cursor.py | Changed cursor.description to return SQL type codes instead of Python types; added _column_metadata storage; updated _build_converter_map to use SQL type codes; added mappings for UDT, XML, DATETIME2, SMALLDATETIME types |
| mssql_python/constants.py | Added SQL_SS_UDT = -151 constant for SQL Server User-Defined Types |
| mssql_python/pybind/ddbc_bindings.cpp | Added C++ constants for SQL_SS_UDT, SQL_DATETIME2, SQL_SMALLDATETIME; implemented UDT handling in SQLGetData_wrap, FetchBatchData, FetchMany_wrap, and FetchAll_wrap for LOB streaming |
| tests/test_004_cursor.py | Added lob_wvarchar_column to test schema; updated test_cursor_description to expect SQL type codes; added comprehensive geography type test suite (14 new tests); separated LOB and non-LOB fetch tests; fixed output converter test for UTF-16LE encoding |
| tests/test_003_connection.py | Simplified converter integration tests to use SQL type constants directly instead of dynamic type detection |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3f52dd7 to
646ef04
Compare
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql_python/connection.pyLines 46-54 Lines 1016-1024 mssql_python/cursor.pyLines 130-138 Lines 154-162 Lines 1128-1136 Lines 1475-1483 Lines 2543-2551 📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.logger_bridge.hpp: 58.8%
mssql_python.pybind.logger_bridge.cpp: 59.2%
mssql_python.pybind.ddbc_bindings.cpp: 69.5%
mssql_python.pybind.ddbc_bindings.h: 69.7%
mssql_python.pybind.connection.connection.cpp: 75.3%
mssql_python.row.py: 79.5%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.pybind.connection.connection_pool.cpp: 79.6%
mssql_python.__init__.py: 84.9%
mssql_python.cursor.py: 85.2%🔗 Quick Links
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
tests/test_004_cursor.py:1
- Changed from catching generic
Exceptionto specificmssql_python.Error. This is more precise and follows best practices for error handling in tests.
"""
tests/test_004_cursor.py:1
- The date '2025-11-26' is in the future. Consider using a past date or current date to avoid potential timezone-related test failures.
"""
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Improve __hash__ error message with helpful guidance for dict key usage - Move spatial tests (geography/geometry/hierarchyid) to test_017_spatial_types.py - Add binary parameter round-trip tests for geography and geometry - Update type stub and unit test for new hash behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
tests/test_004_cursor.py:1
- This change corrects the exception type from a generic
Exceptiontomssql_python.Error, which is more specific and accurate for database errors. This is already fixed in the updated code.
"""
tests/test_004_cursor.py:13063
- Simplified cleanup by removing unnecessary try-except block around DROP TABLE statement. This is already fixed in the updated code.
cursor.execute(f"DROP TABLE IF EXISTS {table_name}")
tests/test_004_cursor.py:1
- Added proper encoding handling for string comparison in output converter test. The driver passes string values as UTF-16LE encoded bytes to output converters, so the test now correctly encodes the comparison value. This is already fixed in the updated code.
"""
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
SqlTypeCode should be used instead of SQLTypeCode. I am assuming this is a public type and the naming would matter more. PascalCase for Python classes is recommended and applicable to acronyms also. |
- Rename class from SQLTypeCode to SqlTypeCode (PascalCase for acronyms) - Fix spatial binary round-trip tests to use Deserialize() instead of STGeomFromWKB() - Update all references in cursor.py, connection.py, __init__.py, type stubs, tests, and docs
Fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add test_018_polars_integration.py with 6 tests verifying polars compatibility - Tests cover: basic types, DATE columns (original failure case), all common types, NULLs, large results - Add polars to requirements.txt for CI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Use pytest.importorskip() to gracefully skip tests when polars cannot be imported due to platform incompatibility (e.g., missing CPU instructions on Alpine ARM64).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Removed 'decimal' and 'mssql_python' imports that were not used in the test module. Addresses Copilot review comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Work Item / Issue Reference
Summary
This PR fixes
cursor.descriptionto properly return SQL type codes per DB-API 2.0 specification, while maintaining backwards compatibility with libraries like pandas and polars that compare against Python types.The original issue was reported when using polars
pl.read_database()which failed with:Key Changes
1. New
SqlTypeCodeClassA dual-compatible type code that compares equal to both:
desc[1] == -9) for DB-API 2.0 compliancedesc[1] == str) for pandas/polars/library compatibility2. SQL Server Spatial Type Support
Added handling for SQL Server User-Defined Types (SQL_SS_UDT = -151):
3. ODBC 3.x Date/Time Type Codes
Added support for:
SQL_TYPE_DATE(91)SQL_TYPE_TIME(92)SQL_TYPE_TIMESTAMP(93)SQL_TYPE_TIMESTAMP_WITH_TIMEZONE(95)4. Hash/Equality Contract Fix
Made
SqlTypeCodeintentionally unhashable since__eq__compares to multiple types with different hash values. Attempting to hash raisesTypeErrorwith a helpful message guiding users to useint(type_code)as dict keys instead.Files Changed
mssql_python/cursor.pySqlTypeCodeclass, updated_initialize_description(), fixed_build_converter_map()mssql_python/constants.pySQL_SS_UDT,SQL_SS_TIME2,SQL_SS_XMLconstantsmssql_python/pybind/ddbc_bindings.cppmssql_python/__init__.pySqlTypeCodemssql_python/mssql_python.pyiSqlTypeCodemssql_python/connection.pySqlTypeCodetests/test_002_types.pySqlTypeCodeunit teststests/test_004_cursor.pytests/test_017_spatial_types.pytests/test_018_polars_integration.pyrequirements.txtpolarsfor integration testingCHANGELOG.mdUsage Examples
SqlTypeCode - Dual Compatibility
Polars Integration (Original Issue)
Spatial Types
Testing
Test Coverage
SqlTypeCodeunit tests_column_metadataPolars Integration Tests
test_polars_read_database_basictest_polars_read_database_with_datestest_polars_read_database_all_common_typestest_polars_read_database_with_nullstest_polars_read_database_large_resulttest_cursor_description_type_code_polars_compatibilityBreaking Changes
None. The
SqlTypeCodeclass maintains full backwards compatibility:cursor.description[i][1] == strstill workscursor.description[i][1] == -9also worksint(cursor.description[i][1])returns raw SQL type codeRelated Issues